docs: add REVIEW.md for KiloCode automated review guidance#104
docs: add REVIEW.md for KiloCode automated review guidance#104anandgupta42 wants to merge 2 commits into
Conversation
Built by mining 113 commits (11 fix commits over 18mo) and ~90 human PR review comments for this repo's real bug classes: pydantic extra="forbid" crashes on new dbt manifest fields, required fields breaking on disabled/ unit-test nodes, version-specific field access on vendored dbt-artifacts- parser types, checksum shape assumptions, mutable default arguments, and API-client failures not surfaced to the CLI user. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewIncremental review of commit Files Reviewed (1 file)
Previous Review Summary (commit 65d0138)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 65d0138)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by glm-5.2 · Input: 18.5K · Output: 3.7K · Cached: 138.6K |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65d01387cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - Unused imports/variables, bare `except`, raise-from style (`RUF`, `RSE`), bugbear patterns (`B`), | ||
| comprehension style (`C4`), pathlib usage (`PTH`), quote style (`Q`), pytest style (`PT`). | ||
| - Naive `datetime` construction — ruff's `DTZ` rule already catches this. | ||
| - Basic `flake8-bandit` (`S`) security smells (e.g. `assert` in non-test code, `eval`/`exec`). |
There was a problem hiding this comment.
This tells Kilo not to flag assert in non-test code because the Bandit S rules supposedly catch it, but pyproject.toml explicitly ignores S101 (flake8-bandit assert). In diffs that add runtime assertions, CI will not report them, and this guidance would also suppress the reviewer from catching code paths that can disappear under python -O; please either remove assert from the never-flag examples or enable the rule.
Useful? React with 👍 / 👎.
| 2. **Fields marked required on node wrapper models** (#98, #95, #96 — `identifier`, | ||
| `resource_type` made optional after crashing on `Disabled*`/unit-test nodes). A field required | ||
| on the "normal" node case often isn't populated on disabled nodes, unit-test nodes, or older | ||
| manifest versions. Flag any new required (non-`Optional`) field added to an `Altimate*` node |
There was a problem hiding this comment.
This equates required fields with non-Optional, but the package uses Pydantic 2 (setup.py:66 requires pydantic >=2.0,<3.0), where identifier: Optional[str] is still required unless it also has a default such as = None. A node wrapper can therefore introduce the same disabled/unit-test missing-field crash while satisfying this guidance, so the reviewer should check for Pydantic-required fields rather than just non-Optional annotations.
Useful? React with 👍 / 👎.
| - Two distinct run modes with different data completeness: a full CLI run (`datapilot dbt | ||
| project-health`) vs. the pre-commit hook, which partially generates manifest/catalog from only | ||
| changed files. Manifest-loading changes need to work under both. | ||
| - Minimum supported Python is 3.7 per README — avoid relying on syntax/stdlib features unavailable |
There was a problem hiding this comment.
This sets Kilo's compatibility floor to Python 3.7 based on README text, but the install metadata is python_requires=">=3.8" in setup.py:63 and the tooling targets py38+ (pyproject.toml:40/:52). With this guidance, the reviewer will flag valid Python 3.8-only code as incompatible with a version users cannot install, creating false positives instead of catching real compatibility regressions.
Useful? React with 👍 / 👎.
| # REVIEW.md — datapilot-cli | ||
|
|
||
| Python CLI (`altimate-datapilot-cli` on PyPI) that parses dbt `manifest.json`/`catalog.json` | ||
| artifacts across dbt manifest schema versions (vendored `dbt-artifacts-parser`, v1–v12) and runs |
There was a problem hiding this comment.
This describes the CLI as handling manifest schemas v1-v12, but DBTFactory.get_manifest_wrapper only creates wrappers for ManifestV10, ManifestV11, and ManifestV12 and raises AltimateNotSupportedError for older manifests. Because later guidance treats crashes on any claimed-supported version as Critical, Kilo will review diffs against v1-v9 shapes that project-health cannot run; please separate parser coverage from insight-wrapper support or narrow this to v10-v12.
Useful? React with 👍 / 👎.
| project-health`) vs. the pre-commit hook, which partially generates manifest/catalog from only | ||
| changed files. Manifest-loading changes need to work under both. |
There was a problem hiding this comment.
The current pre-commit path loads the configured manifest/catalog files in executor_hook.py before calling process_changed_files, and that helper only returns the changed file names; it does not generate partial manifest/catalog objects from those files. This invariant will make Kilo require manifest-loading changes to handle a data shape the active hook no longer produces, while the real requirement is full manifest plus optional/missing catalog.
Useful? React with 👍 / 👎.
|
@kulvirgit — tagging you as one of this repo's most active recent contributors to review. This PR adds a root REVIEW.md: repo-specific guidance for the KiloCode automated reviewer, mined from this repo's own fix-commit history and PR review comments. It is docs-only (one additive file, no code touched). Admin-merge is blocked here by the required-approval rule, so a quick approve + merge would land it — focus areas are in the PR description. Thanks! |
- Scope manifest-version support correctly: raw schema types span v1-v12, but insight-check wrapper support (DBTFactory.get_manifest_wrapper) is v10-v12 only - Stop telling reviewers to suppress `assert` findings — `S101` is ignored in ruff config, so CI does not actually catch assert in non-test code - Clarify Pydantic v2 semantics: `Optional[str]` without a default is still a required field, not automatically optional - Fix Python floor from stale README claim (3.7) to the actually enforced 3.8 (setup.py python_requires, pyproject/tox target-version) - Correct pre-commit hook invariant: it always loads the full manifest/catalog; process_changed_files only narrows selected models, it does not generate partial artifacts - Trim some unrelated focus-area prose to keep the file under 10,000 chars Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecec66e89c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - `eval`/`exec` usage (`flake8-bandit` `S102`/`S307`, still enforced). Exception: `S101` (assert) | ||
| is ignored in `pyproject.toml`, so CI does NOT catch `assert` in non-test code — flag it (it also | ||
| vanishes under `python -O`). |
There was a problem hiding this comment.
For diffs under ci/*, this blanket exception is wrong: pyproject.toml:6-7 ignores all Bandit S rules there, so eval/exec added to a CI helper such as ci/bootstrap.py is not caught by the tox -e check/pre-commit path. This guidance would make Kilo skip exactly those unlinted cases; please scope the exception to files where S102/S307 are actually enforced, or keep reviewing them in ci/*.
Useful? React with 👍 / 👎.
| - Skip deep review of trivial diffs (README/CHANGELOG-only, formatting-only, dependency bumps with | ||
| no vendored-parser impact). |
There was a problem hiding this comment.
Dependency-only PRs that don't touch the vendored parser can still break runtime code: setup.py pins direct runtime dependencies such as sqlglot[c]==30.7.0 (used by the SQL insight optimizer imports) and the supported Pydantic range. Telling Kilo to skip deep review for dependency bumps with no vendored-parser impact would miss incompatible changes to those packages; narrow this exception to truly non-runtime bumps or explicitly keep runtime dependency compatibility in scope.
Useful? React with 👍 / 👎.
1 participant
What this is
REVIEW.mdis repo-specific guidance for the KiloCode automated code reviewer. Kilo reads it fromthe PR base branch, so it activates once this merges to
main(enable Use REVIEW.md in Kilo'srepo settings).
How it was built
Mined this repo's history in one bounded pass: 113 commits, 11 fix commits over the last 18
months, and ~90 human PR review comments. The focus areas are grounded in that evidence, not
generic advice.
Top focus areas
extra="forbid"on manifest/catalog wrapper models — new dbt manifest versions add fieldsthe wrapper doesn't know about yet; forbidding extras crashes parsing on a routine dbt bump.
nodes, and older manifest versions routinely miss fields the "normal" node case has.
deferreddon't exist on everymanifest version's node types (e.g. v12
RPCNode).checksumis a dict on some node types, a string/object onothers.
None—--catalog-pathis optional and can be stalerelative to the manifest.
called out explicitly in past human review.
Also documents known-intentional patterns (e.g. the API client deliberately not raising on non-2xx,
print()used for CLI output) so the reviewer doesn't re-litigate settled decisions, and marks thevendored
dbt_artifacts_parsercopy and release/version-bump plumbing as high-blast-radius.🤖 Generated with Claude Code
Note
Low Risk
Documentation-only change with no runtime, release, or dependency impact.
Overview
Adds REVIEW.md, repo-specific instructions for the KiloCode automated reviewer once merged to
main(with Use REVIEW.md enabled in repo settings).The doc steers reviews toward manifest/catalog correctness and CLI behavior that CI cannot catch—especially Pydantic
extra="forbid"on wrappers, required fields on disabled/unit-test nodes, version-specific field access, checksum shape assumptions, optional catalog paths, and related bug classes mined from past fixes and review comments. It also lists what not to flag (ruff/black/tox coverage, intentional APIprint()behavior, non-raising API client), repo invariants (vendored parser, v10–v12 wrapper scope, Python 3.8), and high-blast-radius paths the bot should not auto-edit.Reviewed by Cursor Bugbot for commit ecec66e. Bugbot is set up for automated code reviews on this repo. Configure here.